Conversation
GitHub Actions
Error
github-actions
Bot
added
kind/protocol
AKolenda
changed the title
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
|
A Linux backend fills a real gap, since the Linux client falls back to the stub today. Two of the design reasons in the description do not hold for the protocols, though, and they drive the parts of this I would push back on. "Neither clipboard protocol notifies a non-owner of selection changes": both ext-data-control-v1 and wlr-data-control-unstable-v1 send a selection event to every data-control client whenever the selection changes, and on X11, XFixes (XFixesSelectSelectionInput) gives the same notification. The 750 ms poll, which re-reads the full text and image on every tick, is a limit of arboard's API rather than of the protocols. "Neither protocol lets a client offer data it only produces on demand": both are on-demand by design. A data-control source gets a send event only when something pastes, and an X11 selection owner answers SelectionRequest only when asked. That is the model MS-RDPECLIP's delayed rendering is built for: announce the formats, then fetch the data only when a local application pastes. Fetching every remote copy as soon as it is announced moves every copied image across the connection whether or not it is ever pasted. The worker's check that it is not reading back its own write is also what On the server side of this same bridge I use data-control directly, with the selection event for change detection and on-demand sources for delayed rendering, which avoids both the polling and the eager transfer. Is there a reason to stay on arboard for the Linux backend rather than using data-control directly, with X11 as the fallback? |
|
You're right on both counts, thanks for spelling them out. The description blamed the protocols for what are really arboard's limits: it only exposes whole-value get and set, so there's no selection event to listen to and no way to serve a paste on demand. That's where the 750 ms poll and the eager fetch come from. There's no reason to stay on arboard beyond getting something working, so I'll move this onto data-control directly (ext-data-control-v1, then wlr-data-control), using the selection event and delayed rendering, with XFixes and selection ownership for X11. The echo check will go through One question before I write that. |
|
Yes, I'll do that, and I'll put it in |
|
Thanks for the patience on this. The data-control clipboard client is up as #2055, as a linux-only data_control module in ironrdp-cliprdr-native. It is the client only, with no CLIPRDR wiring, so the backend in this PR can be built on it. DataControl::connect picks ext-data-control-v1 or wlr-data-control, and read(mime) returns the current selection with charset-tolerant type matching. set_selection takes a Content whose types are either given data or only advertised. A paste of an advertised type without data raises a TransferRequest through on_transfer, and complete() or fail() answers it, which lines up with the Format Data Request and Response. on_change reports selections made by other clients only, so a backend does not see its own clipboard come back. The loop detector stays the backend's concern. GNOME has no data-control protocol, so connect returns Error::Unsupported there, and X11 is not covered, which is where XFixes comes in. Both PRs touch crates/ironrdp-cliprdr-native/Cargo.toml and src/lib.rs, so whichever merges second will need a small rebase. |
Add the data_control module to ironrdp-cliprdr-native so a Linux CLIPRDR backend can read and set the clipboard without a window. It speaks ext-data-control-v1 and wlr-data-control-unstable-v1 and uses whichever the compositor offers. One thread owns the Wayland connection and DataControl is the handle to call from anywhere. A type advertised without data raises a TransferRequest when something pastes it, which maps onto the CLIPRDR Format Data Response. A selection the client set itself is not reported back as a local copy. Reads are capped at 100 MiB and time out when the source stalls for 5 seconds. The state tests live in ironrdp-testsuite-core behind the __test feature, because inline tests are not built for this crate.
Benoît Cortier (CBenoit)
added
automation-failed
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #2016 adds a Linux CLIPRDR backend (arboard-based, X11/Wayland) plus client wiring. The mechanics are largely sound: capability advertisement, format list/data PDU handling, PNG/DIB conversion, thread isolation, and tests check out, and the client gracefully falls back to the stub. The weaknesses are a design whose justification rests on factually incorrect doc-comment claims about X11/Wayland capabilities (driving the 750 ms poll and eager remote fetch the author already agreed to rework onto data-control/XFixes per #2055), a hand-rolled echo guard duplicating ironrdp_cliprdr::loop_detector, a heavy Linux-only dependency stack slated for removal, a duplicated PNG/RGBA codec missing bitmap's allocation guard on a remote-reachable path, a redundant PendingPaste enum, triplicated cfg blocks in the client, and a low-severity protocol hazard: Format Data Responses cannot be correlated to requests, so a late reply after the pending-paste timeout can apply superseded content to the newer…
github-actions
Bot
added
needs-author-action
`ironrdp-cliprdr-native` only had a Windows backend, so clipboard redirection on Linux fell back to the stub and did nothing. `LinuxClipboard` bridges CLIPRDR to the desktop clipboard through `arboard`: the Wayland data-control protocol when the compositor offers it, the X11 clipboard (through XWayland on Wayland desktops) otherwise. Plain text (CF_UNICODETEXT) and images (CF_DIB and CF_DIBV5, converted through PNG with `ironrdp-cliprdr-format`) travel in both directions. Files and HTML are not supported. The OS clipboard is driven from a dedicated thread, so the CLIPRDR callbacks, which arrive on the session task, never wait on it: - Neither clipboard protocol notifies a non-owner of selection changes, so the local clipboard is polled every 750 ms, and only after the server requested the initial format list. - A remote copy is fetched as soon as it is announced and written to the OS clipboard, since neither protocol lets a client offer data it only produces on demand. Reading that write back is not mistaken for a local copy. - A local copy supersedes an outstanding remote response, and a newer remote copy waits for the outstanding one, or replaces it after 5 s. The client uses it for `ClipboardType::Enable` on Linux. A desktop without a usable clipboard logs a warning and uses the stub backend instead of failing the connection. The worker's state machine is tested against a fake clipboard (`cargo test -p ironrdp-cliprdr-native --lib`).
05ed96a to
e6a0cc6
Compare
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
|
Pushed e6a0cc6, rebased onto current master and #2055, addressing all seven review findings. The Linux backend now uses event-driven selection notifications and on-demand rendering with the shared Wayland client and native X11/XFixes support. Follow-up review also fixed ownership-event races, late-response handling, and bounded descriptor/writer/INCR lifetimes. Validation on this exact revision: 1,764 core tests and 103 extra tests pass; all three private X11 integration tests pass, including xclip interoperability, large transfers and backpressure; full workspace Clippy, formatting, changed-file typos, and diff checks pass. Wayland state and real-pipe tests pass; no live compositor interoperability run was performed. Fresh platform CI is running. The shared public API dependency-resolution failure is addressed separately by #2071 and requires that workflow repair on the base branch. |
github-actions
Bot
added
automation-failed
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
|
Follow-up d826491 fixes two lints exposed by building the native crate without the test-only feature: removed the now-unused LocalChanged payload and an obsolete lint expectation. Both production and __test native Clippy configurations pass, as does Clippy for the affected test target. Re-ran all 48 clipboard tests and all three private X11 integration tests successfully; formatting, typo and diff checks also pass. The final normal CI run passed all 27 checks on d826491: https://github.com/Devolutions/IronRDP/actions/runs/36975991083. The separate API check still needs the shared workflow repair in #2071 merged into the base branch. |
Adds native Linux clipboard redirection for Unicode text and images, using the shared Wayland data-control client from #2055 and an X11/XFixes fallback. This PR is stacked on #2055 and current master.
Remote format lists are advertised immediately; contents are requested only when a local application pastes. Local selection changes are event-driven, and local content is read only when the RDP peer requests it. The backend uses the existing clipboard loop detector and bounded bitmap converters, removing the polling arboard backend, direct PNG dependency, and duplicate image codec.
CLIPRDR responses have no request identifier. Only one Format Data Request is outstanding at a time: a timed-out OS paste is released, while the wire request remains reserved until its late response is drained. Selection generations reject obsolete data. If the peer never answers, remote pastes fail until channel reinitialization instead of applying stale content to a later copy.
Wayland and X11 bound pending transfers and use absolute deadlines. X11 supports INCR for large content and isolates reads with separate requestor windows. Ownership acknowledgments and current-owner checks prevent queued events from replacing a newer remote selection. The localized data-control changes also bound held descriptors and active writers and ignore events from replaced sources.
File transfer and HTML are not implemented. Desktops without data-control use X11/XWayland when available, otherwise the existing stub backend.
Validation
helper,__bench, locked dependencies, and warnings denied passes. Workspace formatting, changed-file typo checks, and diff checks pass.